Add mypy to CI pipeline and begin typing modules #435
Conversation
gnikonorov
added
skip-changelog
gnikonorov
changed the title
gnikonorov
force-pushed
the
issue_434_begin_integration
branch
from
dd258ea
to
927096a
Compare
|
Something that worries me a little bit, is that we might be raising the bar/entry for contributors by using typing. 🤔 |
Why do you say that? If anything I'd say it lowers the bar since people know the type of arguments immediately on reading the code and no longer have to guess at what's going on with types Besides, it's becoming more common practice to type Python |
You pretty much answered your own question:
Yes, it's becoming more common, but far from the norm yet, and to be able to read it - they have to first understand it, otherwise it's just noise that often can lead to hard to understand errors. Especially for more complex types. |
|
It's just the typing library, and you only have complex types if you introduce them. In my personal experience I don't think it'll complicate things. Most people are used to types (c++, Java, etc.) and the types are named almost identically to the Python objects they represent. The only issue I can see is someone having a hard time guessing what type something is when writing new code, but I would say that the author should fully understand what they're implementing in that case before they implement it. |
| @@ -20,41 +28,51 @@ def extra(content, format_type, name=None, mime_type=None, extension=None): | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
html(), image() etc are facades to extra().
to make this more clear, i suggest to define a variable with the return type of extra() as _ExtraResult = Dict[str, Optional[str]]
and use this _ExtraResult where needed for extra(), html() and so on
| @@ -1,5 +1,5 @@ | |||
| try: | |||
| from . import __version | |||
| from . import __version # type: ignore | |||
|
|
|||
| __version__ = __version.version | |||
There was a problem hiding this comment.
since __version is an optioanl import at the time typecheckers run,
i usggest to typehint __version__ ala
__version__: str = __version.version this makes type checkers aware that __version__ is supposed to be a sting and warn/fail if it is not
|
Could you contribute here @jeffwright13 ? |
Sure |
src/pytest_html/hooks.py
Outdated
| @@ -1,6 +1,7 @@ | |||
| # This Source Code Form is subject to the terms of the Mozilla Public | |||
| # License, v. 2.0. If a copy of the MPL was not distributed with this | |||
| # file, You can obtain one at http://mozilla.org/MPL/2.0/. | |||
| # type: ignore | |||
There was a problem hiding this comment.
Is this a directive for an IDE extension or something? Like, maybe mypy? I am familiar with typing (use it all the time), but not the details of mypy.
This reverts commit 6f567e7.
There was a problem hiding this comment.
This is a great first step, we can make the config stricter later. I fixed the conflicts while merging, used pyproject.toml instead of setup.cfg and upgraded the mypy version, let me know what you think @BeyondEvil.
Looks great! I don't know what the implications might be, I'm just happy that someone is giving this project some love! 😅 Just make sure you squash the commits before merge. 👍 |
Begin the mypy/typing addition as outlined in #434 by incorporating mypy and typing:
__init__.pyextras.pyoutcome.pyutil.pyThe rest of the files will be typed in follow up PRs to not have to submit one large PR.
The mypy config was started by taking that of pytest's and then being a little stricter on untyped/incomplete function definitions and calls. You can see the full list of config options here, but I think this is a good enough starting point.